Fix findings from a full source review (core 0.3.1, actioncable 0.2.4, client 0.4.3) - #39
Conversation
…, client 0.4.3) Core (Rust): - update_ready? is now EXACT. It only checked the per-client clock lower bound, but yrs's integration gate also requires origin/right-origin/parent blocks (routinely other clients') and post-Skip blocks sit above the lower bound. A cross-client-origin delta on a server missing that client's content passed ready?, then update_advances? misread the parked result as an already-applied retry (pending doesn't move a state vector): the channel ACKED AND DROPPED real content. Reproduced empirically. ready? now trial-integrates on a probe seeded with the doc's integrated state (clock check kept as a cheap pre-filter); advances? gained defense in depth (a parked update reports as advancing, never a duplicate). - read_text deadlock: it opened a second read txn while a chained temporary still held the first; yrs's write-preferring lock made a concurrent writer deadlock the process inside nogvl (uninterruptible). Single txn now. - TOCTOU in integrated_update: pending check and encode ran in separate txns, so a concurrent gappy apply between them could serve pending anyway. One txn now. - update_advances? pre-filter: blocks beyond the doc's SV trivially advance; the common (novel-update) case skips the full O(doc) probe. - read_xml: Lexical linebreak/tab nodes emit \n/\t instead of vanishing. ActionCable concern (Ruby): - A lost-ack retry re-broadcasts before acking :applied. Record-then-crash (or a failed broadcast) previously left live subscribers permanently stale: the retry skipped distribution, and nothing else could reach them. - A missing document key fails closed (Y::Error) instead of silently recording under nil, broadcasting to a dead stream, and still acking (the AnyCable fresh-instance + forgotten-key case). - Floor raised to yrby >= 0.3.1 (the ack-and-drop fix lives in core). Client (TypeScript): - rejected() handler: an auth-rejected subscription surfaces via onError and tears down instead of hanging at "connecting" forever, queueing edits. - Awareness frames are content-validated (dry-run entries) before apply: a partially-malformed payload previously mutated awareness state entry by entry with no event fired. Trailing bytes inside the blob also rejected. - bfcache restore: presence is stashed on pagehide and restored on pageshow(persisted) - a restored page no longer rejoins as a ghost. - Transport sends are guarded: sync throws and promise rejections (@anycable/web) surface via onError instead of unwinding/unhandled. - CJS TypeScript consumers get real CJS-flavored declarations (dist/cjs d.ts + per-condition types exports); fixes TS1479 under node16 resolution. Packaging: - yrby no longer ships the yrby-decoder gem's files (the frozen duplicate could shadow a newer standalone release across the load path). - Cargo.lock ships in the source gem: source builds compile the exact crate graph CI tested. - Demo: the unauthenticated audit-control endpoint (history wipe + per-write delay injection) is no longer mounted in production. New real-Y.js fixture (CrossClientOrigin) + regression tests across all layers, including a read_text-vs-writers thread hammer. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…tests
Answers "did you verify every scenario?" honestly — each fix's regression
test was run against the ORIGINAL buggy code where feasible:
- read_text deadlock (H1): DISCRIMINATES. Against the reverted buggy form the
thread hammer deadlocked so hard the process survived timeout(1)'s SIGTERM
for 7+ minutes (threads stuck in uninterruptible nogvl) and needed SIGKILL;
with the fix it passes in milliseconds.
- awareness half-application (client M8): DISCRIMINATES. Against the reverted
envelope-only validation, exactly the two new tests fail — the failure
message ("the valid entry was NOT half-applied") empirically proves the old
partial-mutation behavior.
- TOCTOU (H2): NOT black-box reproducible — 20k racing iterations couldn't
hit the nanoseconds-wide window even against the buggy two-transaction
form. The new concurrency test is documented honestly as a contention net
(catches lock-skipping/fast-path regressions); the fix's guarantee is
structural (one transaction is atomic under the doc's lock).
New tests this commit:
- protocol.rs: integrated_update concurrency net (writer parking/healing
gappy updates vs reader encoding; every encode must be pending-free).
- test/packaging_test.rb: gemspec file-list regressions (no decoder or
actioncable files in the core gem, essentials + Cargo.lock present, no
tests/artifacts packaged in any gem).
- client: bfcache tests via a window shim — pagehide stashes presence,
pageshow(persisted) restores it, a non-persisted pageshow does not
resurrect stale presence. (This path previously had NO coverage.)
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Verification addendum — every claim re-auditedEach fix's regression test was run against the original buggy code where feasible:
Added in the follow-up commit: bfcache tests (previously zero coverage, via a window shim), gemspec packaging regressions, and the Rust contention net. Totals: Rust 28 / Ruby 98 / client 52, clippy+fmt+rubocop clean. e2e on this branch: CI ran the full demo battery (durability, 4-browser, collab, Falcon, cross-process, AnyCable) — green. Locally ran the CI-excluded suites: crash_recovery ✅, all 4 audit scenarios ✅, stress 3/3 ✅ (~20k msgs, 0 server errors; one cold-start connection flake on the very first run, not reproducible). Known remaining caveat: the linebreak/tab extraction test uses a structure modeled on @lexical/yjs storage, not bytes captured from a live Lexical editor. |
Closing the last review-verification caveat surfaced the real bug: driving a
live Lexxy editor (agent-browser: type "foo", Shift+Enter, type "barbaz") and
running read_xml on the captured durable state returned "foobarbaz" — the
hand-built regression test had modeled the structure wrong. A Lexical
LineBreakNode is NOT an XmlText child with __type=linebreak; it is an embedded
**Y.Map with { __type: "linebreak" }**, the same shape as the per-text-node
metadata maps the walker deliberately skips — so the newline was being
swallowed by the metadata arm.
Fix: Out::YMap embeds now consult their __type — "linebreak" emits '\n',
"tab" emits '\t', anything else (the "text" metadata maps) stays silent, in
both walk_lexical_block and inline_lexical_text. The XmlText-child handling
stays as a defensive path.
Tests:
- fixtures/lexical_linebreak.bin: ground-truth bytes captured from the live
editor via the lexxy-realtime test server (GET /content/:room); the new
test asserts they extract as "foo\nbarbaz".
- The hand-built test now builds the REAL structure (Y.Map embeds, including
a silent metadata map) instead of the imagined one.
Verified end-to-end: Doc#read_xml on the captured bytes returns "foo\nbarbaz".
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Caveat closed — and it caught a real bug. Driving a live Lexxy editor (agent-browser: type Fixed the |
Each comment now states the invariant and why it matters, without the incident narrative (that lives in the CHANGELOG and PR). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…halves The comment simplification cut the clause explaining why the readiness probe seeds with integrated_update rather than the lossless encode. Restore it compactly, and pin both reasons with tests: - a doc carrying legacy pending must still accept unrelated healthy updates (a lossless seed would veto everything), and - a dependency satisfied only by pending content is NOT ready (recording it would put a gap in the durable log). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Two questions about this comment in a row means it wasn't carrying its weight: name the ambiguity (retry vs parked-as-pending both leave the state vector unchanged) and why it matters (false here = ack-and-drop). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Applies the HIGH findings and the important MEDIUMs from a full src review of all components (Rust ext, ActionCable concern, TS client, packaging, demo). Findings that live on the open PRs (#33 live-maps, #38 gap-strike) are intentionally NOT here.
Core — Rust (yrby 0.3.1)
update_ready?is now exact — fixes silent ack-and-drop of real content (empirically reproduced).It only checked the per-client clock lower bound, but yrs's integration gate also requires origin/right-origin/parent blocks (routinely other clients') and post-Skip blocks in merged updates. A cross-client-origin delta on a server missing that client's content passed
ready?;update_advances?then misread the parked result as an already-applied retry (pending doesn't move a state vector) → acked:appliedand dropped. Now: trial-integration on a probe seeded with the doc's integrated state (clock check kept as a cheap pre-filter).update_advances?also gained defense in depth: a parked update reports as advancing, never a duplicate. Regression-tested with a new real-Y.jsCrossClientOriginfixture + a merged-Skip-gap Rust test.read_textprocess-deadlock. A chained temporary held one read txn while acquiring a second; yrs's write-preferring lock deadlocked reader-vs-writer insidenogvl(uninterruptible —kill -9territory). Single txn now, plus a threads-vs-writers hammer test.TOCTOU in gap-free encoding.
has_pending+ encode ran in separate txns; a concurrent gappy apply between them could serve pending anyway. One txn now.Perf:
update_advances?skips its full O(doc) probe when the update carries blocks beyond the doc's SV — the common novel-update case no longer rebuilds the doc. read_xml: Lexical linebreak/tab emit\n/\t(no more"foo⏎bar"→"foobar").ActionCable concern (yrby-actioncable 0.2.4)
:appliedwithout distributing, and nothing else could reach them. Idempotent apply makes the duplicate free.Y::Error) instead of silently recording under nil, broadcasting to a dead stream, and still acking (AnyCable fresh-instance + forgottenkey).yrby >= 0.3.1(the ack-and-drop fix lives in core).Client (yrby-client 0.4.3)
rejected()handler — an auth-rejected subscription surfaces viaonError+ tears down, instead of hanging at "connecting" forever while silently queueing edits.@anycable/web) surface viaonError; reliable frames stay queued.dist/cjs/*.d.ts+ per-conditiontypesexports; fixes TS1479 for CJS consumers under node16 resolution.Packaging / demo
yrbyno longer ships theyrby-decodergem's files (frozen duplicate could shadow the standalone gem across the load path — was latent in published 0.3.0).Cargo.lockships in the source gem (build the crate graph CI tested).Verification
-D warnings+ rustfmt cleantsc --noEmitclean🤖 Generated with Claude Code